chore: Improve Sonos Memory Usage #2306
Conversation
|
Channel deleted. |
Test Results 71 files 455 suites 0s ⏱️ Results for commit a50d490. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 8aff210 |
dljsjr
force-pushed
the
chore/improve-sonos-memory-usage
branch
2 times, most recently
from
fd45493 to
415ad10
Compare
There was a problem hiding this comment.
Looks good, mostly some comments on being a little more memory efficient but I'm not sure how much they really matter.
Do you have any before and after numbers for how much this will save?
I have also been meaning to redo the analysis I did for 57 on 58 and wonder if we will see any changes from the original bonded set changes since that greatly reduced the amount of memory related crashes. I can try and take a look at that tomorrow along with running the change on my own setup.
| return rawget(ret, k) | ||
| end | ||
|
|
||
| local proxy_newindex = function(_, _, _) |
There was a problem hiding this comment.
Just calling out that this only prevents new keys from being set and will still allow modification of current values. I think this is fine since this is just our driver.
| for k, v in pairs(SpeakerDiscoveryInfo) do | ||
| rawset(ret, k, v) | ||
| end |
There was a problem hiding this comment.
I think if we define the metatable outside of the function (per comment below this) then SpeakerDiscoveryInfo could be the metatable and __index could be updated to handle the methods. I don't think it really matters though and this is fine.
There was a problem hiding this comment.
It seemed more straight forward to do it this way and still have the lazily evaluated URL keys.
dljsjr
force-pushed
the
chore/improve-sonos-memory-usage
branch
from
415ad10 to
a50d490
Compare
@NoahCornell did some analysis of driver heap usage, and Sonos was disproportionately above the mean for the heap-space-per-device line fit on the dataset. This PR improves memory usage by limiting the information we keep resident from API responses to the fields that we actually use in regular operation. Additional background: One source of heap usage was unbounded memory growth due to task spawning on a certain error pathway, which he already fixed in a different PR. Another source was the fact that we were cacheing the entire API response for the Player and Group information for the LAN's Sonos topology. These API payloads are pretty large, and have actually gotten larger over time. This has been the case for this driver for a very long time; the decision to store the whole response object was made so that we would have the information available if we needed it in the future for bug fixes or enhancements. It turns out that the information we're utilizing hasn't really changed much over the last few years, so I'm feeling quite comfortable about excising the majority of the payload information at this point. We see pretty signifcant memory savings with these changes, and the savings should scale appreciably with device count, which is a big win.
dljsjr
force-pushed
the
chore/improve-sonos-memory-usage
branch
from
a50d490 to
8aff210
Compare
|
Invitation URL: |
3 participants
Type of Change
Checklist
Depends On
Description of Change
@NoahCornell did some analysis of driver heap usage, and Sonos was
disproportionately above the mean for the heap-space-per-device line fit
on the dataset.
This PR improves memory usage by limiting the information we keep resident
from API responses to the fields that we actually use in regular operation.
Additional background:
One source of heap usage was unbounded memory growth due to task spawning
on a certain error pathway, which he already fixed in a different PR.
Another source was the fact that we were cacheing the entire API
response for the Player and Group information for the LAN's Sonos topology.
These API payloads are pretty large, and have actually gotten larger over time.
This has been the case for this driver for a very long time; the
decision to store the whole response object was made so that we would
have the information available if we needed it in the future for bug
fixes or enhancements.
It turns out that the information we're utilizing hasn't really changed
much over the last few years, so I'm feeling quite comfortable about
excising the majority of the payload information at this point. We see
pretty signifcant memory savings with these changes, and the savings
should scale appreciably with device count, which is a big win.
Summary of Completed Tests
Tested on my personal setup by overriding the driver files directly. Will need to be regression tested by internal QA once this lands on Alpha; the OAuth stuff precludes this from being tested using the PR channel invite.